Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Depricated find and findIndex from IFilteringExpressionsTree, added FilteringUtil #14815

Merged
merged 23 commits into from
Oct 14, 2024
Merged

Depricated find and findIndex from IFilteringExpressionsTree, added FilteringUtil #14815

merged 23 commits into from
Oct 14, 2024

Conversation

Otixa
Copy link
Contributor

@Otixa Otixa commented Sep 24, 2024 •
edited by gedinakova
Loading

Closes #

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

ivanvpetrov
ivanvpetrov requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@ivanvpetrov ivanvpetrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The circular reference in filtering-utils.ts must be addressed

projects/igniteui-angular/src/lib/data-operations/filtering-util.ts Outdated Show resolved Hide resolved
ivanvpetrov
ivanvpetrov previously approved these changes Sep 24, 2024
View reviewed changes
gedinakova
gedinakova requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@gedinakova gedinakova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a section about this breaking change in the CHANGELOG.md. For more info, refer to CONTRIBUTING.md > "Breaking changes and migrations" section.

@gedinakova gedinakova added ❌ status: awaiting-test PRs awaiting manual verification filtering labels Sep 26, 2024
@Otixa
Copy link
Contributor Author

Otixa commented Sep 26, 2024

Adding a migration is not currently possible for this scenario.
Created #14833

@gedinakova gedinakova mentioned this pull request Sep 30, 2024
Open
14 tasks
damyanpetev
damyanpetev previously approved these changes Oct 11, 2024
View reviewed changes
@gedinakova gedinakova added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Oct 11, 2024
@gedinakova
Copy link
Contributor

@Otixa @damyanpetev Guys, I'm a little concerned of the fact we already have a FilterUtil class in filtering-strategy.ts and now we introduce FilteringUtil. If we decide to leave them both I believe we need to add some docs what these are.
image

@Otixa In addition, I guess FilteringUtil should be imported from "igniteui-angular", not from "igniteui-angular/src/lib/data-operations/filtering-util".

@damyanpetev
Copy link
Member

@gedinakova Good catch! Also (facepalm), got stuck with this util being internal for some reason, but on the off chance it is useful to someone... yeah.

Have to wonder if the FilterUtil was exposed by mistake - tracing it back to an refactor part of another feature and both don't seem to be related to 'expose util to run filter standalone' in some manner. ¯\(ツ)/¯
Regardless, it's out there now, so we either integrate into it (two filter utils sounds off to me) or we renamed the new util and export (ExpressionsTreeUtil? takers?).

@gedinakova gedinakova added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Oct 14, 2024
@gedinakova gedinakova self-assigned this Oct 14, 2024
@gedinakova gedinakova merged commit 4fc69a9 into master Oct 14, 2024
5 checks passed
@gedinakova gedinakova deleted the mcherkasov/deprecate-find branch October 14, 2024 11:52
This was referenced Nov 6, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gedinakova gedinakova gedinakova approved these changes

@damyanpetev damyanpetev damyanpetev approved these changes

@ivanvpetrov ivanvpetrov Awaiting requested review from ivanvpetrov

Assignees

@gedinakova gedinakova

Labels
filtering version: 18.2.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Otixa @gedinakova @damyanpetev @ivanvpetrov